-
Notifications
You must be signed in to change notification settings - Fork 186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use INITIALISE subworkflow instead of WorkflowMain.initialise() #2430
Conversation
…n.initialise() Changes: - Guts WorkflowMain, particularly the initialise() method - Replaces it with INIITIALISE subworkflow - Should be easier to maintain via nf-core/modules (more regular updates) - Uses more native Nextflow for bioinformatics developers to follow Should incidentally fix nf-core#2289
a502001
to
cf14aa1
Compare
@@ -59,6 +50,9 @@ include { CUSTOM_DUMPSOFTWAREVERSIONS } from '../modules/nf-core/custom/dumpsoft | |||
// Info required for completion email and summary | |||
def multiqc_report = [] | |||
|
|||
// Get map of parameter values | |||
def summary_params = paramsSummaryMap(workflow) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could possibly add this to the INITIALISE subworkflow to make everything central.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
INITIALISE is meant to be used in the main.nf
isn't it? I think it will be difficult to use it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could work anywhere, but we would have to re-work structure slightly for the scoping to be correct for this particular use case. But it's hardly a massive problem having it here anyway. It would be cleaner to not have anything outside the workflow scope in this file, e.g. global scope stuff only happens in ./main.nf
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see! sounds good then :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove even more the workflow to be honest, but that's for the next release, we're too early to move so fast I'm afraid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you like to add this modification if it is a fast change and merge the PR for this release?
Codecov Report
@@ Coverage Diff @@
## dev #2430 +/- ##
==========================================
- Coverage 71.07% 71.06% -0.02%
==========================================
Files 87 87
Lines 9431 9431
==========================================
- Hits 6703 6702 -1
- Misses 2728 2729 +1
|
|
||
// Print help message if needed | ||
if (params.help) { | ||
def logo = NfcoreTemplate.logo(workflow, params.monochrome_logs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The subworkflow INITIALISE does not contain the nf-core logo, we should add it or keep it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added it back, but it now always appears. Options:
- Leave it in
main.nf
, let a pipeline developer handle it as they want - Add to INITIALISE, with an optional flag so it only appears with --help or pipeline run (a bit annoying with the monochrome logs stuff, but doable).
For now, adding NfcoreTemplate.logo
to ./main.nf
is probably 'good enough'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with adding it to ./main.nf
.
The only problem I can see is with retrieving the version from CLI, but to be fair it was already printing some text that must be stripped, so it doesn't look as a big change to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just having a look now and as long as you switch around the order of functions correctly I think it will be fine(ish).
@@ -59,6 +50,9 @@ include { CUSTOM_DUMPSOFTWAREVERSIONS } from '../modules/nf-core/custom/dumpsoft | |||
// Info required for completion email and summary | |||
def multiqc_report = [] | |||
|
|||
// Get map of parameter values | |||
def summary_params = paramsSummaryMap(workflow) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
INITIALISE is meant to be used in the main.nf
isn't it? I think it will be difficult to use it here.
Nextflow.error("Please provide an input samplesheet to the pipeline e.g. '--input samplesheet.csv'") | ||
} | ||
} | ||
|
||
{%- if igenomes %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about the igenomes bit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't touched igenomes - you still need to use WorkflowMain.getGenomeAttribute
for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since igenomes is optional in case you are creating a customised template, I think the best is to keep it in this groovy function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am on a one man crusade to remove everything from lib/
. But yes, I'm not sure it's appropriate within INITIALISE. Haven't got a good solution for this yet 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good
My personal feeling is we should merge this one after the next release and upcoming hackathon (Oct 2023). That way we can spend some time refining it before the next major release. Do you agree @maxulysse @mirpedrol? |
No longer needed |
Changes:
Guts WorkflowMain, particularly the initialise() method
Replaces it with INIITIALISE subworkflow
Should be easier to maintain via nf-core/modules (more regular updates)
Uses more native Nextflow for bioinformatics developers to follow
Should incidentally fix https://github.com/nf-core/tools/issues/2289>
PR checklist
CHANGELOG.md
is updateddocs
is updated